-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
USWDS-Site - Prettier should run on .md files. #3037
base: main
Are you sure you want to change the base?
Conversation
Doesn't require a build
…rt of gems are excluded from prettier.
3a82e12
to
89e0ace
Compare
e429e0f
to
89e0ace
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cathybaptista good work so far. Confirming that this runs formatting checks. Added comments for improvements.
Please don't forget that we'll also need a commit, or a few, that address the formatting errors so the test can pass.
Running the script locally results in 400+ files changed. We should consider fixing section by section, like all components in a commit, about changes, utils, patterns, etc etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -69,6 +69,9 @@ jobs: | |||
- run: | |||
name: Build site assets | |||
command: npm run build:all-assets | |||
- run: | |||
name: Check formatting | |||
command: npm run prettier:check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This check formatting works, but the build
job doesn't seem like the most appropriate place because:
- The build doesn't rely on formatting being correct
- If there are formatting errors, we won't know until it's deep into the job
- We don't know what exactly is being checked
You can see in the workflow that it takes over 2 minutes to check for formatting.
Consider separating it and running before build
.
Summary
We were ignoring running prettier on markdown files. This can cause inconsistencies and errors when writing MD/Frontmatter.
Example: 1c6453d
Related issue
Note comment:
#2868 (comment)
These are links to these files, there weren't any problems running prettier
pages/design-principles/overview.md (before)
pages/design-principles/overview.md (after)
https://github.com/uswds/uswds-site/blob/main/pages/documentation/maturity-model.md (before)
https://github.com/uswds/uswds-site/blob/cb-enable-prettier-on-markdown/pages/documentation/maturity-model.md (after)
Closes #2868
Problem statement
By not running prettier against .md files (in .prettierignore), we are not ensuring these files are subject to coding standards and proper formatting.
Solution
Fix issues.
Major changes
Added new script to config.yml:
name: Check formatting
command: npm run prettier:check
Prettier check in package.json now checks .md files
lint in package.json runs "npx gulp lintJS lintSass && npm run prettier",
run prettier runs both prettier:md && prettier:scss
Do not run prettier on the vendor directory since we don't manage those md files.
Pipeline before running prettier:
Pipeline after running prettier:
Testing and review